Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[api] delete file #1122

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
Open

[api] delete file #1122

wants to merge 23 commits into from

Conversation

MalinAhlberg
Copy link
Contributor

@MalinAhlberg MalinAhlberg commented Nov 7, 2024

Related issue(s) and PR(s)
This PR closes #1134 .

Description
This PR adds the delete file functionality to the api component. Specifically, it deletes the file from the inbox and it adds a new file log event, setting the file status to disabled.

Also, it adds the fileID to the list functionality of the api, since that field is needed in order to delete a file:

curl -H "Authorization: Bearer $token" "http://localhost:8090/users/[email protected]/files" | jq .
[
  {
    "fileID": "d32f7117-bb85-40e2-9c52-101bf9c1ca5a",
    "inboxPath": "test_dummy.org/race_file.c4gh",
    "fileStatus": "ready",
    "createAt": "2024-11-19T10:50:07.836513Z"
  },
  {...

How to test
make build-all then PR_NUMBER=$(date +%F) docker compose -f .github/integration/sda-s3-integration.yml run integration_test.
List the files (eg with http://localhost:8090/users/[email protected]/files) and make sure files in the inbox can be deleted, and that archived files can not be deleted.

@MalinAhlberg MalinAhlberg force-pushed the feature/api-delete-file branch 8 times, most recently from b31bc38 to 446a1ff Compare November 21, 2024 10:22
@MalinAhlberg MalinAhlberg marked this pull request as ready for review November 21, 2024 10:22
@MalinAhlberg MalinAhlberg requested a review from a team November 21, 2024 10:25
Copy link
Contributor

@kostas-kou kostas-kou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job. I have only some minor comments

sda/cmd/api/api.go Outdated Show resolved Hide resolved
sda/internal/database/database.go Show resolved Hide resolved
.github/integration/tests/sda/60_api_admin_test.sh Outdated Show resolved Hide resolved
sda/cmd/api/api.md Show resolved Hide resolved
@MalinAhlberg
Copy link
Contributor Author

MalinAhlberg commented Nov 27, 2024

Great comments @kostas-kou ! Fixed most of them in 92903a6, but left this one for others to see <- solved during stand-up.

kostas-kou
kostas-kou previously approved these changes Nov 28, 2024
pahatz
pahatz previously approved these changes Nov 28, 2024
Copy link
Contributor

@pahatz pahatz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Especially on the tests, very extensive.
I don't have any significant remark on the PR.

sda/cmd/api/api.go Outdated Show resolved Hide resolved
sda/internal/database/db_functions.go Show resolved Hide resolved
@MalinAhlberg
Copy link
Contributor Author

@kostas-kou and @pahatz, thanks for your reviews! I have fixed the weird comment, rebased on main and also rebased to get rid of the fixup-commits. Only 3ac7e5e is new, the rest is the same as when you reviewed.

@MalinAhlberg
Copy link
Contributor Author

...and added 8a745c4 for the rbac

pahatz
pahatz previously approved these changes Nov 29, 2024
aaperis
aaperis previously approved these changes Dec 2, 2024
Copy link
Contributor

@aaperis aaperis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't tested, but it looks good. Tiny minor comments :-)

sda/cmd/api/api.md Outdated Show resolved Hide resolved
sda/cmd/api/api.go Outdated Show resolved Hide resolved
sda/cmd/api/api.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@jbygdell jbygdell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More specific comments are coming

@MalinAhlberg MalinAhlberg force-pushed the feature/api-delete-file branch 2 times, most recently from 109e7e2 to d690e54 Compare December 3, 2024 07:43
@MalinAhlberg
Copy link
Contributor Author

Updated, fixed, rebased etc. Please re-review.

.github/integration/tests/sda/60_api_admin_test.sh Outdated Show resolved Hide resolved
.github/integration/tests/sda/60_api_admin_test.sh Outdated Show resolved Hide resolved
.github/integration/tests/sda/60_api_admin_test.sh Outdated Show resolved Hide resolved
.github/integration/tests/sda/60_api_admin_test.sh Outdated Show resolved Hide resolved
.github/integration/tests/sda/60_api_admin_test.sh Outdated Show resolved Hide resolved
sda/cmd/api/api.go Show resolved Hide resolved
sda/cmd/api/api.go Outdated Show resolved Hide resolved
sda/cmd/api/api.go Show resolved Hide resolved
sda/cmd/api/api.go Outdated Show resolved Hide resolved
sda/cmd/api/api.go Outdated Show resolved Hide resolved
"submission_user= $1 and id = $2 " +
"AND EXISTS (SELECT 1 FROM " +
"(SELECT event from sda.file_event_log where file_id = $2 order by started_at desc limit 1) " +
"as subquery WHERE event = 'uploaded')"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this condition, the first retrieved query will has the event status error for files with fileStatus = error. In this case, no filepath will be returned. And therefore, finally get the error File could not be found in inbox when trying to delete a file with status error.
In fact, only files with the status uploaded can be deleted.

Copy link
Contributor

@nanjiangshu nanjiangshu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, @MalinAhlberg. I do have one question regarding which types of files can be deleted. If I understand correctly, we decided that all files that are not archived can be deleted. However, in the current setup, it seems that only files with the uploaded fileStatus can be deleted. Please see my inline comment for further clarification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[api] delete files from inbox
7 participants